Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix vnode DOM not existing #509

Closed
wants to merge 2 commits into from
Closed

Fix vnode DOM not existing #509

wants to merge 2 commits into from

Conversation

LuKks
Copy link

@LuKks LuKks commented Jul 12, 2023

I'm getting this error:

Uncaught (in promise) TypeError: Cannot read properties of null (reading '__P')
    at index.js+app+app:57:20
    at Array.forEach (<anonymous>)
    at Object.replaceComponent (index.js+app+app:56:10)

I couldn't trace on how to minimally reproduce the error but it happens consistently

It could be an edge case bug or maybe it's my naive way of implementing live reload, it's more or less like so:

const reload = useImporter(file.href)

const original = await import(reload.url + FILE_POSTFIX)
const previous = await reload.import(false)
let uncached

try {
  uncached = await reload.import(true)
} catch (error) {
  await handleEdgeCases(error)
}

const modules = Object.keys(original)
  .concat(Object.keys(previous))
  .filter(uniques)

for (const name of modules) {
  if (!uncached[name]) continue

  window.__PREFRESH__.replaceComponent(original[name], uncached[name], true)
  window.__PREFRESH__.replaceComponent(previous[name], uncached[name], true)
}

I'm always resetting the state there, and I didn't implement the signature comparison because it's too complex, but this has been working perfectly

@changeset-bot
Copy link

changeset-bot bot commented Jul 12, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: b621c1b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@prefresh/core Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

JoviDeCroock
JoviDeCroock reviewed Jul 13, 2023
View reviewed changes
Copy link
Member

@JoviDeCroock JoviDeCroock left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason why something like this could happen is becauase you are invoking replaceComponent on a VNode which has exited the DOM. This is something you are doing as you are invoking replaceComponent twice in the above. Removing the invocation where you remove previous and just doing replace(new, old) it should work correctly.

EDIT: the tests fail as well for non-resetting behavior

@LuKks
Copy link
Author

LuKks commented Aug 20, 2023

Tried to remove the second replace but it's the first one that fails replaceComponent(original[name], uncached[name], true). I also tried the reverse, and other combinations

The first time ever that the code gets executed fails so in this case there is no double replace or anything like that

This issue only happens on a specific file, so in all other files it's fine. To defend the double replaceComponent, if I remember correctly it's required in case someone changes the exports

Btw you said replace(new, old) but I'm using it like replace(old, new) which I think it's the correct API. Your way doesn't throw but it doesn't update the component

@LuKks
Copy link
Author

LuKks commented Aug 20, 2023 •
edited
Loading

I think the PR is still valid because it should not throw if possible, this literally fixes the issue and it allows the component to update correctly. It just that it would be cool to know exactly what is happening haha

Thanks for the help!

@LuKks
Copy link
Author

LuKks commented Sep 27, 2023

CI error seems unrelated because same one happened on main branch recently. Maybe if we re-run the CI several times

This was referenced Oct 1, 2023
Merged
Closed
@JoviDeCroock
Copy link
Member

Superseded by #514 apologies for the flaky tests, created an issue for that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoviDeCroock JoviDeCroock JoviDeCroock left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@LuKks @JoviDeCroock